Import Event Dispatcher and tests. r=gmarty #115
Conversation
| * @param {string} eventName Name of the event to check listeners for. | ||
| * @returns {boolean} | ||
| */ | ||
| hasListeners(eventName) { |
There was a problem hiding this comment.
I have mixed feelings about this method - but it simplifies a lot my next PR, so I've added this one. If in the future I find better approach to know that I can shutdown the bus (or something that depends on the bus) if there is no listeners anymore, I would probably try to get rid of this method.
|
@gmarty r? |
| @@ -169,7 +157,8 @@ export default class EventDispatcher { | |||
|
|
|||
| /** | |||
| * Removes all registered listeners for the specified event. | |||
| * @param {string} eventName Name of the event to remove all listeners for. | |||
| * | |||
| * @param {string} [eventName] Name of the event to remove all listeners for. | |||
There was a problem hiding this comment.
nit: Do the brackets around the param name for optional? If we use Closure Compiler flavour of JSDoc, you can indicate optional params with a = (see https://developers.google.com/closure/compiler/docs/js-for-compiler#types):
/**
* Removes all registered listeners for the specified event.
*
* @param {string=} eventName Name of the event to remove all listeners for.
*/There was a problem hiding this comment.
Btw, does webstorm + enabled ESLint recognize {string=} as optional parameter for you?
There was a problem hiding this comment.
It works for me on WebStorm but I don't think I set up ESLint properly. If it's a bug maybe we should fill a ticket in WebStorm or ESLint.
There was a problem hiding this comment.
Yeah, likely a bug somewhere, will file one if not forget :)
|
Note on |
| @@ -0,0 +1,423 @@ | |||
| import EventDispatcher from 'js/lib/foxbox/event-dispatcher'; | |||
|
|
|||
| describe('EventDispatcher >', function () { | |||
There was a problem hiding this comment.
question: Do we have a good reason to use function () { as opposed to () => { in the test files?
There was a problem hiding this comment.
Probably not, "historically" I used function in tests and where I don't need to grab context (likely browser knows that and doesn't do anything especial, but I don't know for sure), so just used to do it.
Do you think we should use arrows here?
There was a problem hiding this comment.
It's just an idea.
That would result in a more readable file, but that's debatable.
There was a problem hiding this comment.
Okay, I'll keep like it's currently, but we can discuss this later, I don't have a preference actually
|
r+ when you add a test case for |
Yeah, maybe |
Ha-ha, I knew you'll notice that :) Will add few tests for this new method as well.
Almost for free :p It required some routine |
|
Nits fixed, travis is green - merging! |
No description provided.